-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Only load the telementry service as a background task if used #3085
Conversation
… used to improve the startup duration #2966
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides minor comments: LGTM
@@ -54,6 +54,8 @@ | |||
} | |||
|
|||
private <T extends JabRefDialog> void trackDialogOpening(Class<T> clazz) { | |||
Globals.getTelemetryClient().trackPageView(clazz.getName()); | |||
if (Globals.getTelemetryClient()!=null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checkstyle will hit you! 😈
I think, this should be converted to an Optional, shouldn't it? Then the usual Globals.getTelemtryClient().ifPresent(client -> client....)
@chochreiner my macbook has not yet arrived , so I cant test it on MacOs. |
The changes work fine on MacOS. I've just added you, bc you have added the telemetry part and I might have overlooked sth, which should also use the "null" checks. |
The failing tests seem to be about the Help pages, otherwise LGTM |
Go ahead merging, I checked the new commit. The Optional.ofNullable in the
getter should not be a performance issue
Am 07.08.2017 22:04 schrieb "Christoph" <notifications@github.com>:
… The failing tests seem to be about the Help pages, otherwise LGTM
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#3085 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABTafipNcNM5YIBli7CLWwU1dUq26SAhks5sV23EgaJpZM4Ovzyq>
.
|
telemetryClient.flush(); | ||
if (Globals.prefs.shouldCollectTelemetry()) { | ||
getTelemetryClient().ifPresent(client -> client.trackSessionState(SessionState.End)); | ||
getTelemetryClient().ifPresent(client -> client.flush()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you have only a single method call with zero or one parameter, you can do some syntactic sugar by doing ifPresent(TelementryClient::flush)
just for information ;)
https://docs.oracle.com/javase/tutorial/java/javaOO/methodreferences.html
Up to now, the telemetry service was always running (even if the reporting was turned of).
This initialization already requires a lot of time during startup, which is already rather long on MacOS
I hope I have caught all usages of the telemetry client to avoid nullpointer exceptions, but i would be great if @lynyus could have a look at it.
gradle localizationUpdate
?